There's a little more to excise in the endowments logic, but removing the helper is a good start to prevent benign usage.

Yeah this is not supported. First, a function is not passable. And even if we had a Far function, no guest created capability (aka remotable or promise, not copy data) can currently be passed back. The main problem is that when you pass a guest created remotable, there is no guarantee the host will only use that capability before the flow invocation completes. Ideally we'd need a way for the author to make explicit that the capability is no longer expected to be used, aka a revocation mechanism, and it would be an error of the flow to complete without explicitly revoking these ephemeral remotables first. The problem with promises is somewhat similar: we could allow them but enforce that the promise is settled by the flow by the time the flow completes. Without that it would be useless to the host.

Would something like provideAccountForChain(orch.getChain('agoric')) work?

If you want to define this logic collocated, I'm wondering if we'd be able to use the orchestrateAll trick (likely with some changes in endowments needed), aka await atomicProvider.provideAsync('localAccount', flows.makeLocalAccount).

This all seems very clunky though, and I'm not sure how to make this cleaner. Is this a generic pattern that we need to support, or something we could have a helper built into orch (which would raise the question of how the orch state is shared between multiple orchestrated flows)?

If I understand what's going on here, this likely does not work on restart, and is upgrade hazardous for other reasons.

Afaik, provideAsync lazily invokes the maker if the value is not found in the store (nor pending).

orchestrate is an exo definition that needs to be rewired during start for every subsequent incarnation.

Given that the maker given to provide here contains the orchestrate call, and that it only happens once, the orchestrated flow will never be rewired in future incarnations, preventing the vat from upgrading.

Furthermore, the await here is for a non immediate promise. It is either dependent on network or on other vats, and would prevent the start result promise from immediately settling during the first crank. The only reason it works right now is because the first incarnation of the contract has this logic, and there is a limitation in zcf which cannot enforce single crank first start of contract (because the first zcf start is actually done in a second crank). If this was added as new logic in a second incarnation of the contract, the upgrade to that incarnation would fail.

I believe the following would work:

Then of course you still need to await the sharedLocalAccount inside flow, but there is just no way around that.

I imagine a slightly more reusable approach would be:

  const orchAccountsProvider = zone.subZone('orchAccounts');
  const { makeOrchAccount } = orchestrateAll(sharedFlows, {});
  const provideOrchAccount = chain => orchAccountProvider.makeOnce(chain, makeOrchAccount);

Then provideOrchAccount can be given as an endowment. It's return value of course must still be awaited in flows.

Great review! 🙏

Blah I need to get back to my type mapping work.

Is this comment necessary? Maybe you are envisioning some shared helpers that will rely on add'l endowments in the future?

A @file comment might be helpful here. Perhaps something to the effect of:

Shared Flows are common async flow functions that can be reused across contracts. They are bound to a particular contract's context via orchestrateAll. See ./send-anywhere.contract.js for example usage.

Since this is an example contract, consider over communicating what's going on here:

This doesn't seem to be the correct place for this - we should only care about the return signature of deposit in local-orchestration-account.js. We currently use the returnVoidWatcher (handler) there, so I believe this commit can be removed

Thanks for the catch. I revised to fix the type annotation instead